Refactor search caching to store normalized data by JPrevost · Pull Request #465 · MITLibraries/timdex-ui

JPrevost · 2026-09-04T18:27:21Z

Why are these changes being introduced:

We currently store raw data from Primo and Timdex, which is larger than necessary.
Load More functionality started to introduce a new caching mechanishm, but it still fell back on the raw data caches and was only in place for the All tab.
Our cache is regular hitting the max data storage size for our redis tier, and rather than expanding it further (more money), it felt worth considering options to store data more efficiently.

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/USE-701

How does this address that need:

Refactors flow to compute and check for normalized search data prior to running external queries.
Includes ADR documenting this change.

Developer

Accessibility

ANDI or WAVE has been run in accordance to our guide.
This PR contains no changes to the view layer.
New issues flagged by ANDI or WAVE have been resolved.
New issues flagged by ANDI or WAVE have been ticketed (link in the Pull Request details above).
No new accessibility issues have been flagged.

New ENV

All new ENV is documented in README.
All new ENV has been added to Heroku Pipeline, Staging and Prod.
ENV has not changed.

Approval beyond code review

UXWS/stakeholder approval has been confirmed.
UXWS/stakeholder review will be completed retroactively.
UXWS/stakeholder review is not needed.

Additional context needed to review

E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.

Code Reviewer

Code

I have confirmed that the code works as intended.
Any CodeClimate issues have been fixed or confirmed as
added technical debt.

Documentation

The commit message is clear and follows our guidelines
(not just this pull request message).
The documentation has been updated or is unnecessary.
New dependencies are appropriate or there were no changes.

Testing

There are appropriate tests covering any new functionality.
No additional test coverage is required.

Why are these changes being introduced: * We currently store raw data from Primo and Timdex, which is larger than necessary. * Load More functionality started to introduce a new caching mechanishm, but it still fell back on the raw data caches and was only in place for the All tab. * Our cache is regular hitting the max data storage size for our redis tier, and rather than expanding it further (more money), it felt worth considering options to store data more efficiently. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-701 How does this address that need: * Refactors flow to compute and check for normalized search data prior to running external queries. * Includes ADR documenting this change.

coveralls · 2026-09-04T18:29:03Z

Coverage Report for CI Build 33914782201

Coverage increased (+0.02%) to 98.324%

Details

Coverage increased (+0.02%) from the base build.
Patch coverage: 60 of 60 lines across 1 file are fully covered (100%).
No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	1730
Covered Lines:	1701
Line Coverage:	98.32%
Coverage Strength:	83.73 hits per line

💛 - Coveralls

qltysh · 2026-09-04T18:29:18Z

❌ 8 blocking issues (12 total)

Tool	Category	Rule	Count
rubocop	Lint	Method has too many lines. [11/10]	3	❌
rubocop	Lint	Assignment Branch Condition size for `fetch\_timdex\_data` is too high. [<6, 16, 4> 17.55/17]	3	❌
rubocop	Lint	Class has too many lines. [392/100]	1	❌
rubocop	Lint	Cyclomatic complexity for `prepare\_timdex\_query` is too high. [8/7]	1	❌
qlty	Structure	Function with high complexity (count = 10): build_primo_cache_payload	3
qlty	Structure	Function with many parameters (count = 4): build_primo_cache_payload	1

qltysh · 2026-09-04T18:29:20Z

+
+    cached.merge(pagination: pagination)
+  rescue StandardError => e
+    { results: [], pagination: {}, errors: handle_primo_errors(e), show_continuation: false, hits: 0 }


Method has too many lines. [11/10] [rubocop:Metrics/MethodLength]

qltysh · 2026-09-04T18:29:20Z

+    return cached.merge(pagination: {}) if cached[:errors]
+
+    pagination = Analyzer.new(@enhanced_query, cached[:hits], :timdex).pagination
+    cached.merge(pagination: pagination)


Assignment Branch Condition size for fetch_timdex_data is too high. [<6, 16, 4> 17.55/17] [rubocop:Metrics/AbcSize]

qltysh · 2026-09-04T18:29:20Z

+    end
+  end

+  def build_primo_cache_payload(primo_response, results, hits, offset)


Function with many parameters (count = 4): build_primo_cache_payload [qlty:function-parameters]

qltysh · 2026-09-04T18:29:20Z

-      hits: hits }
-  rescue StandardError => e
-    { results: [], pagination: {}, errors: handle_primo_errors(e), show_continuation: false, hits: 0 }
+    { results: results, errors: errors, show_continuation: show_continuation, hits: hits }


Found 2 issues:

1. Function with high complexity (count = 10): build_primo_cache_payload [qlty:function-complexity]

2. Method has too many lines. [11/10] [rubocop:Metrics/MethodLength]

qltysh · 2026-09-04T18:29:20Z

-        errors: raw.errors.details.to_h
-      }
-    end
+    query


Found 3 issues:

1. Function with high complexity (count = 7): prepare_timdex_query [qlty:function-complexity]

2. Assignment Branch Condition size for prepare_timdex_query is too high. [<8, 9, 14> 18.47/17] [rubocop:Metrics/AbcSize]

3. Cyclomatic complexity for prepare_timdex_query is too high. [8/7] [rubocop:Metrics/CyclomaticComplexity]

Copilot

🟡 Changes recommended

A remaining raw TIMDEX cache path (query_timdex) conflicts with the documented normalized caching strategy and can still populate un-namespaced provider-shaped cache entries.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR refactors SearchController caching so single-source Primo and TIMDEX result requests cache application-normalized payloads (rather than raw provider responses), reducing Redis storage pressure and avoiding repeated normalization work on cache hits. It also adds an ADR documenting the new caching approach and tests asserting that cache hits bypass external calls and normalization.

Changes:

Cache normalized payloads for single-source Primo/TIMDEX searches with an explicit cache namespace and TTL.
Update controller flow so cache lookup occurs before provider calls/normalization, and pagination is computed after cache retrieval.
Add controller tests to ensure cache hits avoid external provider calls and normalizer instantiation; add ADR documenting the decision.

File summaries

File	Description
app/controllers/search_controller.rb	Introduces normalized-results cache namespace/TTL and refactors Primo/TIMDEX fetch paths to cache normalized payloads before external calls/normalization.
test/controllers/search_controller_test.rb	Adds tests asserting that repeated requests hit cache and avoid re-calling external search/normalizers for Primo and TIMDEX.
docs/architecture-decisions/0003-cache-normalized-search-results.md	Documents the decision and expected caching flows for normalized single-source results and all-tab load-more state.

Review details

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Note: geodata and non-geodata timdex queries follow a slightly different process due to geodata loading aggregations and filters. When we refactor the view logic to bring geodata up to date visually, we may want to also consider normalizing how all timdex queries (geo or otherwise) flow through the controller.

qltysh · 2026-09-04T19:04:42Z

+      else
+        { results: [], errors: errors, hits: 0 }
+      end
    end


Found 3 issues:

1. Function with high complexity (count = 8): cached_timdex_data [qlty:function-complexity]

2. Assignment Branch Condition size for cached_timdex_data is too high. [<9, 17, 6> 20.15/17] [rubocop:Metrics/AbcSize]

3. Method has too many lines. [18/10] [rubocop:Metrics/MethodLength]

Copilot

🟡 Changes recommended

The refactor introduces a couple of correctness/efficiency issues in the caching/keying and filter configuration handling that should be addressed before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

app/controllers/search_controller.rb:232

cached_primo_data builds its cache key from @enhanced_query, but Enhancer only sets :tab when the request includes a tab param. That means logically identical all-tab requests (/results?q=... vs /results?q=...&tab=all) can generate different cache keys and store duplicate normalized payloads, reducing cache efficiency (the primary goal of this PR). Consider always keying Primo caches with the resolved @active_tab (or defaulting to 'all') so equivalent requests share the same entry.

Files reviewed: 4/4 changed files
Comments generated: 1
Review effort level: Lite

Filtering out blank entries makes the default behavior (no reordering) work correctly.

Copilot

🟡 Changes recommended

A confirmed geospatial query branching bug can prevent combined geobox+geodistance searches from using the intended query path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

app/controllers/search_controller.rb:334

In execute_geospatial_query, the combined geobox+geodistance branch checks query[:geodistance], but QueryBuilder sets geospatial flags on string keys (e.g., query['geodistance']). This makes the “both enabled” branch effectively unreachable and can route combined requests to the geobox-only query.

Files reviewed: 4/4 changed files
Comments generated: 1
Review effort level: Lite

Copilot

🔵 Needs a closer look

It changes core search caching behavior across Primo/TIMDEX and GeoData paths, so a final human review is warranted to validate runtime/cache-shape impacts beyond the included tests.

Review details

Files reviewed: 4/4 changed files
Comments generated: 0 new
Review effort level: Lite

JPrevost requested a lite review from Copilot September 4, 2026 18:29

qltysh Bot reviewed Sep 4, 2026

View reviewed changes

Copilot started reviewing on behalf of JPrevost September 4, 2026 18:29 View session

mitlib temporarily deployed to timdex-ui-pi-use-701-re-kffghu September 4, 2026 18:30 Inactive

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread app/controllers/search_controller.rb Outdated

JPrevost temporarily deployed to timdex-ui-pi-use-701-re-kffghu September 4, 2026 19:03 Inactive

qltysh Bot reviewed Sep 4, 2026

View reviewed changes

Freeze string constant

eed7fde

JPrevost temporarily deployed to timdex-ui-pi-use-701-re-kffghu September 4, 2026 19:05 Inactive

JPrevost requested a lite review from Copilot September 4, 2026 19:11

Copilot started reviewing on behalf of JPrevost September 4, 2026 19:11 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread app/controllers/search_controller.rb

JPrevost added 2 commits September 4, 2026 15:38

Ensure primo cache key includes tab

6a052ee

Filter out blank ACTIVE_FILTERS

5387341

Filtering out blank entries makes the default behavior (no reordering) work correctly.

JPrevost temporarily deployed to timdex-ui-pi-use-701-re-kffghu September 4, 2026 19:41 Inactive

JPrevost requested a lite review from Copilot September 4, 2026 19:57

Copilot started reviewing on behalf of JPrevost September 4, 2026 19:58 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread app/controllers/search_controller.rb

Align Primo search tab usage with cache key

9deed36

JPrevost deployed to timdex-ui-pi-use-701-re-kffghu September 4, 2026 20:09 View deployment

JPrevost requested a lite review from Copilot September 4, 2026 20:09

Copilot started reviewing on behalf of JPrevost September 4, 2026 20:10 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Uh oh!

Conversation

JPrevost commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Developer

Accessibility

New ENV

Approval beyond code review

Additional context needed to review

Code Reviewer

Code

Documentation

Testing

Uh oh!

coveralls commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 33914782201

Coverage increased (+0.02%) to 98.324%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

qltysh Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ 8 blocking issues (12 total)

Uh oh!

Uh oh!

qltysh Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

qltysh Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

qltysh Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

qltysh Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qltysh Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

qltysh Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JPrevost commented Sep 4, 2026 •

edited

Loading

coveralls commented Sep 4, 2026 •

edited

Loading

qltysh Bot commented Sep 4, 2026 •

edited

Loading